Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow resetting the status of multiple migrations #5287

Open
wants to merge 2 commits into
base: 11.x
Choose a base branch
from

Conversation

DieterHolvoet
Copy link
Contributor

This PR adds the all and tag options to the migrate:reset-status command, together with the ability to pass a comma separated list of migration ids as argument.

@DieterHolvoet
Copy link
Contributor Author

One thing I wasn't sure about is whether we should log Migration x is already Idle if passing the all or tag options, or whether we should only log migrations that were actually reset.

@weitzman
Copy link
Member

One thing I wasn't sure about is whether we should log Migration x is already Idle if passing the all or tag options, or whether we should only log migrations that were actually reset.

Yes lets omit those log messages. Less noise.

@DieterHolvoet DieterHolvoet force-pushed the feature/migrate-reset-multiple branch from eb49a02 to 6e3be62 Compare December 14, 2022 18:39
@DieterHolvoet
Copy link
Contributor Author

Changed and rebased.

Copy link
Member

@claudiu-cristea claudiu-cristea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but we need to enrich the tests to cover also the --all and --tag cases.

throw new \Exception(dt('You must specify --all, --tag, or one or more migration names separated by commas'));
}

if (!$list = $this->getMigrationList($migrationIds, $options['tag'])) {
Copy link
Member

@claudiu-cristea claudiu-cristea Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've already assign $options['tag'] to $tags. Why not use that var?

$status = $migration->getStatus();
if ($status !== MigrationInterface::STATUS_IDLE) {
$migration->setStatus(MigrationInterface::STATUS_IDLE);
$this->logger()->success(dt('Migration @id reset to Idle', ['@id' => $migrationId]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Migration @id status reset to idle (added the "status" word)

$migration->setStatus(MigrationInterface::STATUS_IDLE);
$this->logger()->success(dt('Migration @id reset to Idle', ['@id' => $migrationId]));
$tags = $options['tag'];
$all = $options['all'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if some are using:

  • --all with --tag and some migration names?
  • but --all with --tag?
  • or --all with a list of migrations?

Maybe we should remove --all and NO MIGRATIONS == ALL? Then:

  • no arguments, no options: All migrations
  • only --tag: All migrations filtered by provided tags
  • migration IDs + --tag: should return error (or filter the list on tags?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants